[Recombee] Add internalAdditionalData field to actions #2856
Conversation
mstieranka
force-pushed
the
recombee-internal-additional-data
branch
from
73c6f5e to
15264d7
Compare
| function payloadToBookmark(payload: Payload): AddBookmark { | ||
| return new AddBookmark({ | ||
| userId: payload.userId, | ||
| itemId: payload.itemId, | ||
| timestamp: payload.timestamp, | ||
| recommId: payload.recommId, | ||
| additionalData: { | ||
| ...(payload.internalAdditionalData ?? {}), | ||
| ...(payload.additionalData ?? {}) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Hi @mstieranka thanks for raising this PR.
My main question regards the addition of these new wrapper functions. Why are they needed?
For example why can the additional work of merging the new internalAdditionalData into the additionalData object not be done in the AddBookmark class?
There was a problem hiding this comment.
It can be done in the class, that's a good point. I kept it this way so that it conforms exactly to our API schema, but since this is a specialized API implementation and not a generalized API client (which I'm used to developing), I guess it can be moved into the classes.
There was a problem hiding this comment.
Thanks @mstieranka - it might be neater to have the logic all in one place here if possible. Are you OK updating it please?
This PR adds a new field,
internalAdditionalData, to all applicable actions within the Recombee destination.The goal is for us to be able to track the types of events that are sent using our destination, and being able to use this data in our analytics, as well as assist our customers with any issues they may be having.
We set this new field up with
unsafe_hiddenset totrue, as end users should never be able to modify the values within this field. If this parameter doesn't prevent the user from changing the values within this object, or if there's a better way to obtain$.typeand$.eventwithin actions, please let us know.Additionally, the original
additionalDatafield (that should be present to all users) has had itsdefaultObjectUIset tokeyvalue:only, as we want to discourage users from setting the value of this field to be an entire object.Testing
These changes were tested end-to-end to ensure the event data is sent in all applicable actions, and unit test snapshots were modified to account for the above-mentioned changes.
The new field is not required, meaning this is not a breaking change.